Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add aes-encryption for backups #97

Merged
merged 16 commits into from
Oct 23, 2024
Merged

Add aes-encryption for backups #97

merged 16 commits into from
Oct 23, 2024

Conversation

ostempel
Copy link
Collaborator

@ostempel ostempel commented Oct 7, 2024

References

Closes #31.

References:

Additional Description

Because database can contain sensible data it should be encrypted. Adds aes-encryption to the default workflow.

Release Notes

Breaking Change

NONE

Required Actions

NONE

@ostempel ostempel requested a review from a team as a code owner October 7, 2024 08:41
@ostempel ostempel requested a review from Gerrit91 October 7, 2024 08:41
Copy link
Contributor

@majst01 majst01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some first comments

README.md Outdated Show resolved Hide resolved
cmd/internal/backup/providers/gcp/gcp_integration_test.go Outdated Show resolved Hide resolved
cmd/internal/backup/providers/s3/s3_integration_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Gerrit91 Gerrit91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good first PR. I have not yet finished with the review but already commit some thoughts because I cannot finish today.

cmd/internal/backup/providers/common/common.go Outdated Show resolved Hide resolved
cmd/internal/backup/providers/common/common.go Outdated Show resolved Hide resolved
cmd/internal/backup/providers/common/common.go Outdated Show resolved Hide resolved
cmd/internal/backup/providers/common/common_test.go Outdated Show resolved Hide resolved
cmd/internal/encryption/encryption.go Outdated Show resolved Hide resolved
cmd/internal/encryption/encryption.go Outdated Show resolved Hide resolved
@ostempel ostempel marked this pull request as draft October 8, 2024 09:32
@ostempel ostempel marked this pull request as ready for review October 8, 2024 12:28
cmd/internal/encryption/encryption.go Outdated Show resolved Hide resolved
cmd/internal/encryption/encryption.go Outdated Show resolved Hide resolved
cmd/internal/encryption/encryption.go Outdated Show resolved Hide resolved
cmd/internal/encryption/encryption.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/internal/encryption/encryption.go Outdated Show resolved Hide resolved
cmd/internal/encryption/encryption_test.go Outdated Show resolved Hide resolved
cmd/internal/encryption/encryption_test.go Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
cmd/internal/backup/providers/contract.go Outdated Show resolved Hide resolved
cmd/internal/encryption/encryption.go Outdated Show resolved Hide resolved
cmd/internal/encryption/encryption.go Outdated Show resolved Hide resolved
cmd/internal/encryption/encryption.go Outdated Show resolved Hide resolved
cmd/internal/encryption/encryption.go Outdated Show resolved Hide resolved
return fmt.Errorf("unable to decrypt backup: %w", err)
}
}
i.log.Info("restoring unencrypted backup with configured encryption - skipping decryption...")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This log line seems to be wrong because it's always shown. No matter if it was encrypted or not.

Comment on lines 235 to 236
e.log.Info("read %d bytes: %s", strconv.Itoa(n), err)
break
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be more secure to do this?

Suggested change
e.log.Info("read %d bytes: %s", strconv.Itoa(n), err)
break
return fmt.Errorf("error reading from file (%d bytes read): %w", strconv.Itoa(n), err)

Comment on lines 180 to 181
e.log.Info("read %d bytes: %s", strconv.Itoa(n), err)
break
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as below.

Comment on lines 169 to 179
needsBackup, err := i.db.Check(ctx)
if err != nil {
return fmt.Errorf("unable retrieve backup versions: %w", err)
return fmt.Errorf("unable to check data of database: %w", err)
}

latestBackup := versions.Latest()
if latestBackup == nil {
i.log.Info("there are no backups available, it's a fresh database. allow database to start")
if !needsBackup {
i.log.Info("database does not need to be restored")
return nil
}

i.log.Info("database potentially needs to be restored, looking for backup")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to increase availability of the DB we should do this check before validating the backups.

@Gerrit91 Gerrit91 requested a review from majst01 October 23, 2024 13:22
@Gerrit91 Gerrit91 merged commit 2a69363 into master Oct 23, 2024
2 checks passed
@Gerrit91 Gerrit91 deleted the feature/encryption branch October 23, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

optionally support encryption
3 participants